Skip to content

oauth/xai: pin discovery endpoints to trusted auth hosts and reject userinfo - #4094

Merged
lidge-jun merged 1 commit into
devfrom
codex/xai-oauth-endpoint-validation
Sep 9, 2026
Merged

oauth/xai: pin discovery endpoints to trusted auth hosts and reject userinfo#4094
lidge-jun merged 1 commit into
devfrom
codex/xai-oauth-endpoint-validation

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Closes oauth/xai: validateXaiEndpoint accepts any *.x.ai subdomain and URLs with embedded userinfo #4048. Hardens validateXaiEndpoint in src/oauth/xai.ts — the trust check applied to the OIDC discovery response whose token_endpoint then receives the refresh_token in a POST body (refresh) and the authorization code + PKCE verifier (exchange).
  • Accepted hosts are now pinned to a trusted set — auth.x.ai (the live issuer per today's discovery document) and accounts.x.ai (the second host the reference Grok CLI flow uses) — replacing the previous *.x.ai suffix match that would have accepted any subdomain.
  • URLs with embedded userinfo (https://u:p@auth.x.ai/...) are rejected; previously new URL() preserved userinfo and fetch would have turned it into an Authorization: Basic header on the token request. The rejection message carries only the parsed lowercase host, never the raw URL, so embedded credentials cannot reach logs.
  • Malformed endpoint strings now throw a generic Error ("unparseable endpoint URL") instead of leaking a bare TypeError; explicit ports are rejected. The error type stays Error so the terminal-OAuth classification in src/oauth/index.ts is unaffected.
  • Scope note: fetch follows redirects by default, so this pins the discovery-advertised initial endpoints; it is not a final-destination guarantee against redirects issued by the trusted host itself. Redirect policy is deliberately unchanged and recorded as follow-up scope.
  • Stack (manual chain, merge bottom-up): layer 1 is oauth/xai: honor Retry-After and stop retrying aborted token requests #4087 (retry/abort fixes, base dev); this PR is layer 2, base codex/xai-oauth-retry-after. Review only this layer's diff (the tip commit). Retarget to dev after oauth/xai: honor Retry-After and stop retrying aborted token requests #4087 lands.
  • Eight new regression tests drive the exported discoverXaiOAuthEndpoints with stubbed discovery payloads (accept live shape + accounts.x.ai; reject http, apex x.ai, unlisted subdomains, userinfo — asserting the message does not echo it — explicit port, malformed input).

Verification

  • NOT RUN (local, user restriction): bun test, bun run typecheck, bun run privacy:scan. The userinfo test fixture is written as a split string literal ("https://u:p@" + "auth.x.ai/...") specifically so the privacy scan's email pattern does not match it; this was verified by review against scripts/privacy-scan.ts, not by a local run.
  • Remote CI (this PR, exact head): ci.yml PR jobs — Linux test, macOS platform-macos, gates (tsc --noEmit), plus the hygiene/enforce-target gates. Windows and macos-control run only on the cumulative final-head lane=all dispatch before merge.
  • Security review: the fix design and tests were audited by independent read-only reviewers (two rounds; final verdict PASS-equivalent with one fixture blocker folded). Existing discovery stubs in tests/oauth/ and tests/server/ all use auth.x.ai and keep passing (verified by inspection; CI confirms).

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed. (No user-facing docs change; pre-merge security design notes stay in gitignored scratch per repository policy, the merged diff is the public record.)
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. (This PR IS the auth-surface hardening; rejection messages are credential-redacted.)

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner September 9, 2026 07:27
@coderabbitai

coderabbitai Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)
  • ^dev$
  • ^preview$

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c2ebf893-9ae3-46a8-ac37-eafdf89f43f4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-09T07:32:07.540878Z 6bdfad3 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@lidge-jun

lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

@/home/box/developer/review-comments/pr-4094.md

@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@lidge-jun

lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 71 / 80

이 PR은 xAI OAuth discovery가 돌려준 token_endpoint/authorization_endpoint를 믿는 지점을 좁힙니다. 지금 dev HEAD 4b379b9ec(package 2.49.0, tip #3914 OrcaRouter sponsor)의 src/oauth/xai.ts validateXaiEndpoint는 https이면서 호스트가 x.ai이거나 *.x.ai면 통과합니다. 그래서 임의 서브도메인(evil.x.ai)과 https://u:p@auth.x.ai/... 형태(userinfo → fetch가 Authorization Basic으로 바꿈)가 이론상 들어올 수 있고, 실패 메시지에 raw URL이 그대로 실릴 수 있습니다. discovery 자체는 고정 URL https://auth.x.ai/.well-known/openid-configuration에서 TLS로 가져오므로 실제 공격 면적은 작지만, refresh_token·authorization code·PKCE verifier가 POST로 나가는 마지막 관문이라 allow-list를 좁히는 편이 맞습니다.

이 레이어(스택 2층)는 discovery 응답의 초기 엔드포인트만 핀합니다. 신뢰 호스트를 auth.x.ai/accounts.x.ai로 고정하고, userinfo·명시 포트·파싱 실패를 막습니다. 거절 메시지는 소문자 host만 넣고 credentials를 로그에 안 남깁니다. 스택은 수동 체인입니다. 아래층 #4087(Retry-After/abort, base dev)이 아직 OPEN이고, 이 PR base는 codex/xai-oauth-retry-after입니다. #4087이 랜딩한 뒤 dev로 retarget하세요. 같은 이슈 #4048을 겨냥한 기여자 PR #4065(chilung-cgu, base dev)도 아직 OPEN이라 중복입니다. types/config 분할과 무관하고, #4048을 닫는 인증 표면 hardening입니다.

라인 src/oauth/xai.ts · XAI_TRUSTED_AUTH_HOSTS - suffix 매치를 버리고 allowlist로 바꿉니다. apex x.ai와 미등록 서브도메인을 함께 거절합니다.
라인 validateXaiEndpoint · username/password/port - embedded userinfo와 명시 포트를 막습니다. 메시지는 host만 노출해 privacy:scan·로그 누출을 피합니다.
라인 validateXaiEndpoint · try/catch URL - 잘못된 문자열을 TypeError 대신 generic Error로 바꿉니다. src/oauth/index.ts의 terminal()은 XaiTokenRequestError oauthError만 보므로 Error 유지가 맞습니다.
라인 tests/providers/xai/xai-oauth-retry.test.ts - discover 스텁 8개가 accept/reject·userinfo 비에코·malformed를 잠급니다. 픽스처를 문자열 이어붙이기로 나눈 것도 privacy 스캔 회피로 타당합니다.
경로 스택 - #4087이 열려 있어 지금 dev에 단독 머지할 수 없습니다. 리뷰는 tip 커밋만 보고, 머지는 bottom-up입니다.
경로 중복 #4065 - 같은 #4048 수정이지만 기여자 쪽은 hygiene unsponsored_surface로 막힌 이력이 있습니다. 메인테이너 스택(#4087→이 PR)을 살리고 #4065는 landed/superseded로 닫는 편이 깔끔합니다.
경로 redirect - fetch 기본 redirect를 그대로 두어 “신뢰 호스트가 보낸 redirect 최종지”는 보장하지 않습니다. PR이 후속 범위로 적어 둔 점이 맞습니다.

메인테이너의 판단이 필요한 지점

  • #4087과 이 PR을 같은 열차로 묶을지, #4087만 먼저 넣을지
  • 기여자 fix(oauth): restrict xAI discovery endpoint host and reject userinfo #4065/#4060을 메인테이너 스택에 흡수한 뒤 close할지
  • redirect 최종지 고정(manual redirect / disallow)을 바로 후속 이슈로 열지
  • accounts.x.ai를 영구 allowlist로 둘지, 라이브 discovery만 보고 줄일지

너의 추천
보안 가치가 커서 우선순위는 높습니다. #4087이 먼저 머지되고 이 PR이 dev로 retarget·CI 그린 뒤에 넣으세요. 단독으로 base를 dev에 억지 올리지 말고, 랜딩 후 #4065는 Landed via #<landing> 패턴으로 닫고, redirect 후속은 별도 이슈로 남겨도 됩니다.

이 댓글은 grok-bot이 작성했습니다

@lidge-jun lidge-jun mentioned this pull request Sep 9, 2026
Merged
3 tasks
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087 retry-after) (6b…
4a69bd5 
…dfad3)
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 (carries #4087) (6bdfad3)
577c280
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 (carries #4087) (6bdfad3)
508dd79
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 (carries #4087) (6bdfad3)
ef61775
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint (carries #4087) (6bdfad3)
f41c099
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087) (6bdfad3)
b0fba49
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087) (6bdfad3)
d0e4a32
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087) (6bdfad3)
f083e50
lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087) (6bdfad3)
f1e8e1b
@Ingwannu Ingwannu mentioned this pull request Sep 9, 2026
Merged
3 tasks
Ingwannu
Ingwannu reviewed Sep 9, 2026
View reviewed changes

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the two-file child delta at 6bdfad3 against parent aa9e30c.

The discovered authorization and token URLs are validated before browser authorization, code exchange, or refresh consumes them. The host allowlist, HTTPS requirement, userinfo rejection, and generic parse-error path are consistent with that initial-endpoint boundary. The current public issuer metadata still advertises auth.x.ai for both endpoints. This does not validate a later HTTP redirect destination; the PR now correctly limits its claim.

Two wording precisions for the handoff: URL parsing normalizes an explicit default :443 to an empty port, so this rejects non-default ports, not every explicitly written port. Also, rejecting userinfo is appropriate without relying on the claim that fetch necessarily converts it to Basic authentication.

Exact-head CI 34323954866 passed; Windows/macos-control were skipped. No additional code blocker found, but integration remains held for the parent's landing/retarget and the promised cumulative lane=all evidence. I have not tested against a live credential or merged this stack.

lidge-jun added a commit that referenced this pull request Sep 9, 2026
@lidge-jun
integrate: #4094 xai endpoint pinning (carries #4087) (6bdfad3)
791492a
Base automatically changed from codex/xai-oauth-retry-after to dev September 9, 2026 10:55
@lidge-jun

lidge-jun commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Maintainer integration into dev at exact head 6bdfad3a4f06cdfea92e014ab0ef0b3f8073f00b. Current maintain/admin authority and outstanding maintainer review state were verified with the repository helper. Exact-head Cross-platform CI: https://github.com/lidge-jun/opencodex/actions/runs/34323954866 (success; conditional skipped jobs are not counted as passing tests). The owner explicitly requested immediate serial integration of the prepared PR set. Cumulative lane=all has not yet run; full validation will run on final dev after landing. Independent technical/security review and source carry verification were completed for this delivery. Local product tests/typecheck/build/install were NOT RUN under the owner restriction. Merge uses the original commits and preserves attribution.

@lidge-jun
lidge-jun merged commit 3057078 into dev Sep 9, 2026
31 checks passed
@lidge-jun
lidge-jun deleted the codex/xai-oauth-endpoint-validation branch September 9, 2026 10:55
This was referenced Sep 9, 2026
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ingwannu Ingwannu Ingwannu left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lidge-jun @Ingwannu